Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move ICS Feed button from Ilios Calendar to Main Navigation #7811

Merged

Conversation

michaelchadwick
Copy link
Contributor

Fixes ilios/ilios#4116.

The old functionality is to click on Calendar in the main navigation, click the ICS Feed button, and then click the "Copy My ICS Link" button in the flyout to copy the URL to the user's clipboard.

ilios-ics-pre

The new functionality is to click the ICS Feed button to copy the URL to the user's clipboard, directly from the main navigation, always available, and without the flyout.

ilios-ics-post

Tests for this are still being figured out, so this is a draft for now.

@michaelchadwick
Copy link
Contributor Author

I wasn't able to figure out how to test the new tooltip notification upon clicking the ICS Feed button, but I removed some testing code that was no longer relevant.

@michaelchadwick michaelchadwick marked this pull request as ready for review May 7, 2024 23:15
Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think putting this here was my idea, but now that it's here I don't love it. It feels like too prominent a location for this and doesn't balance particularly well with the other buttons. Some discussion needed for sure. Maybe in the profile menu?

The tooltip copy works, but I think we have some better options. Take a look at:

  1. The copy button on a virtual learning URL in an offering
  2. The copy button on the API key generator at the bottom of the user profile page

I didn't look too deeply at the code as is, think we need to work out some UX first.

@michaelchadwick
Copy link
Contributor Author

The API key copy notification is nice, yes, and could be used instead. As for the placement of the icon itself, I don't have a strong feeling one way or another. It looks like the user profile menu is pretty bare and could easily incorporate more functionality. However, would users know to look there for this?

@michaelchadwick
Copy link
Contributor Author

All righty: I changed the button to have an IliosTooltip on mouse hover, and the click success triggers a flash message instead.

Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you have the tooltip pop out to the right instead of underneath when there is space? If this ends up being more than an hour's work never mind. I'd also like to see the button switch to a check after it is clicked along with the confirmation.

@michaelchadwick
Copy link
Contributor Author

Moved the tooltip so it displays to the right. If there's no room, then it falls back to bottom.

As for the icon itself turning into a checkmark, you mean you want the copy button to go from RSS Feed (pre-click)->Checkmark (clicked)->RSS Feed (post-click)?

@jrjohnson
Copy link
Member

Code looks good, assigning to @dartajax for another visual pass and merge.

@stopfstedt stopfstedt removed their request for review July 18, 2024 18:59
@dartajax
Copy link
Member

dartajax commented Jul 18, 2024

percy won't let me merge this one either but quick question - it looks like the .ics feed button is snuggling up to the Calendar button - they are related but is this visual representation intentional? I am cool with it just wondering

@dartajax
Copy link
Member

we need to be aware - other support people as well as me - that when a user clicks Calendar, the screen shifts and the top menu is not viewable meaning that they may not be able to find the .ics feed button since it has been moved to the top of the screen - just pointing this out

@michaelchadwick
Copy link
Contributor Author

...it looks like the .ics feed button is snuggling up to the Calendar button - they are related but is this visual representation intentional? I am cool with it just wondering

@dartajax The visual representation is intentional per ilios/ilios#4116 (comment).

@michaelchadwick
Copy link
Contributor Author

we need to be aware...that when a user clicks Calendar, the screen shifts and the top menu is not viewable meaning that they may not be able to find the .ics feed button since it has been moved to the top of the screen

@dartajax This is a good point. I'm not sure how often people use this icon link, so not sure if it matters, though. @jrjohnson and @stopfstedt, thoughts?

@michaelchadwick michaelchadwick force-pushed the frontend-4116-add-ics-to-main-nav branch from 9b787ce to 8b7e10f Compare July 18, 2024 20:44
@dartajax
Copy link
Member

we need to be aware...that when a user clicks Calendar, the screen shifts and the top menu is not viewable meaning that they may not be able to find the .ics feed button since it has been moved to the top of the screen

@dartajax This is a good point. I'm not sure how often people use this icon link, so not sure if it matters, though. @jrjohnson and @stopfstedt, thoughts?

My $.02 is that most students grab their feed from here so a lot of people use it. But we are in summer right now and new students can have a new interface and updated instructions on grabbing their feeds.

@jrjohnson
Copy link
Member

I think the scrolling display is a different issue, we don't need to consider it here. Percy has now run, just needs someone to check and make sure all the changes are valid.

@dartajax dartajax merged commit 4522f1a into ilios:master Jul 19, 2024
35 of 36 checks passed
@michaelchadwick michaelchadwick deleted the frontend-4116-add-ics-to-main-nav branch July 21, 2024 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants